Deduplicate repo+sysroot syncfs logic

This is a followup to https://github.com/ostreedev/ostree/pull/3504/commits/6e5a27a29d33d50a2a4380c406405435d919b6b4
which I believe is correct as is. However, we already have a file
descriptor open for the ostree repo, which *must* be on
the same filesystem as `/sysroot/ostree` (the deployment
code forces hardlinking today).

It's hence cleaner to reuse that extant fd instead of opening
a new one - we know we did writes to that fd.

But going farther here, there already is logic to use syncfs
for the repo when downloading objects (in a common case
we actually syncfs twice).

Since these are really the same operation, unify them:

- Add journaling to the repo one syncfs case
- Change the sysroot case to just call it
- Since we log consistently to the journal for all syncfs/fsfreeze
  operations now, drop the SyncStats bits which was a way
  to add info about that to a later journal message

Additionally, let's add an extra check when we're
opening the repo that it's on the same device just on general
principle.

Signed-off-by: Colin Walters <walters@verbum.org>

diff --git a/src/libostree/ostree-repo-commit.c b/src/libostree/ostree-repo-commit.c
index 4bf845c40bd68ef75f6c596f0c00761b9ae4256d..36240faea7d647a6a4cd983aa0655cdccee17ede 100644 (file)
--- a/src/libostree/ostree-repo-commit.c
+++ b/src/libostree/ostree-repo-commit.c
@@ -2227,14 +2227,8 @@ ostree_repo_commit_transaction (OstreeRepo *self, OstreeRepoTransactionStats *ou
   if ((self->test_error_flags & OSTREE_REPO_TEST_ERROR_PRE_COMMIT) > 0)
     return glnx_throw (error, "OSTREE_REPO_TEST_ERROR_PRE_COMMIT specified");
 
-  /* FIXME: Added OSTREE_SUPPRESS_SYNCFS since valgrind in el7 doesn't know
-   * about `syncfs`...we should delete this later.
-   */
-  if (!self->disable_fsync && g_getenv ("OSTREE_SUPPRESS_SYNCFS") == NULL)
-    {
-      if (syncfs (self->tmp_dir_fd) < 0)
-        return glnx_throw_errno_prefix (error, "syncfs(repo/tmp)");
-    }
+  if (!_ostree_repo_syncfs (self, error))
+    return FALSE;
 
   if (!rename_pending_loose_objects (self, cancellable, error))
     return FALSE;

diff --git a/src/libostree/ostree-repo-private.h b/src/libostree/ostree-repo-private.h
index 3ea1ff65a6bcf9395944a571ad3a27d51ba500b0..77ea2a6bccd253ffc6845856a42cc347491ff8de 100644 (file)
--- a/src/libostree/ostree-repo-private.h
+++ b/src/libostree/ostree-repo-private.h
@@ -394,6 +394,7 @@ gboolean _ostree_repo_load_file_bare (OstreeRepo *self, const char *checksum, in
                                       GError **error);
 
 gboolean _ostree_repo_update_mtime (OstreeRepo *self, GError **error);
+gboolean _ostree_repo_syncfs (OstreeRepo *self, GError **error);
 
 gboolean _ostree_repo_add_remote (OstreeRepo *self, OstreeRemote *remote);
 gboolean _ostree_repo_remove_remote (OstreeRepo *self, OstreeRemote *remote);

diff --git a/src/libostree/ostree-repo.c b/src/libostree/ostree-repo.c
index 770846a8630c2b3a5256e97fda096ae532aeede8..3f2f0419d5cf3349273a73e6f27d1bf5bd69a079 100644 (file)
--- a/src/libostree/ostree-repo.c
+++ b/src/libostree/ostree-repo.c
@@ -1521,6 +1521,32 @@ _ostree_repo_update_mtime (OstreeRepo *self, GError **error)
   return TRUE;
 }
 
+gboolean
+_ostree_repo_syncfs (OstreeRepo *self, GError **error)
+{
+
+  if (self->disable_fsync)
+    return TRUE;
+  /* FIXME: Added OSTREE_SUPPRESS_SYNCFS since valgrind in el7 doesn't know
+   * about `syncfs`...we should delete this later.
+   */
+  if (g_getenv ("OSTREE_SUPPRESS_SYNCFS") != NULL)
+    return TRUE;
+
+  gboolean is_system = ostree_repo_is_system (self);
+  if (is_system)
+    ot_journal_print (LOG_INFO, "Starting syncfs for system repo");
+  guint64 start_msec = g_get_monotonic_time () / 1000;
+  int repo_dfd = ostree_repo_get_dfd (self);
+  if (syncfs (repo_dfd) != 0)
+    return glnx_throw_errno_prefix (error, "syncfs(repo)");
+  guint64 end_msec = g_get_monotonic_time () / 1000;
+  if (is_system)
+    ot_journal_print (LOG_INFO, "Completed syncfs() for system repo in %" G_GUINT64_FORMAT " ms",
+                      end_msec - start_msec);
+  return TRUE;
+}
+
 /**
  * ostree_repo_get_config:
  * @self:

diff --git a/src/libostree/ostree-sysroot-deploy.c b/src/libostree/ostree-sysroot-deploy.c
index 4abd7001b944fadfbca493db468689d050324d86..3bfc8d9f52cc2194abdebd0bdcc281aaf112ae3b 100644 (file)
--- a/src/libostree/ostree-sysroot-deploy.c
+++ b/src/libostree/ostree-sysroot-deploy.c
@@ -1628,44 +1628,31 @@ fsfreeze_thaw_cycle (OstreeSysroot *self, int rootfs_dfd, GCancellable *cancella
   return TRUE;
 }
 
-typedef struct
-{
-  guint64 root_syncfs_msec;
-  guint64 boot_syncfs_msec;
-} SyncStats;
-
 /* sync /ostree and /boot which may be separate mount points.
  */
 static gboolean
-full_system_sync (OstreeSysroot *self, SyncStats *out_stats, GCancellable *cancellable,
-                  GError **error)
+full_system_sync (OstreeSysroot *self, GCancellable *cancellable, GError **error)
 {
   GLNX_AUTO_PREFIX_ERROR ("Full sync", error);
-  ot_journal_print (LOG_INFO, "Starting syncfs() for /ostree");
-  guint64 start_msec = g_get_monotonic_time () / 1000;
-  glnx_autofd int ostree_dfd = -1;
-  if (!glnx_opendirat (self->sysroot_fd, "ostree", TRUE, &ostree_dfd, error))
-    return glnx_throw_errno_prefix (error, "glnx_opendirat(/ostree)");
-  if (syncfs (ostree_dfd) != 0)
-    return glnx_throw_errno_prefix (error, "syncfs(/ostree)");
-  guint64 end_msec = g_get_monotonic_time () / 1000;
-  ot_journal_print (LOG_INFO, "Completed syncfs() for /ostree in %" G_GUINT64_FORMAT " ms",
-                    end_msec - start_msec);
-
-  out_stats->root_syncfs_msec = (end_msec - start_msec);
 
   if (!_ostree_sysroot_ensure_boot_fd (self, error))
     return FALSE;
-
   g_assert_cmpint (self->boot_fd, !=, -1);
+
+  // Must have been initialized
+  g_assert (self->repo);
+  // Because we require that /ostree is on the same fs as /ostree/repo, reuse
+  // the logic to syncfs the repo fd.
+  if (!_ostree_repo_syncfs (self->repo, error))
+    return FALSE;
+
   ot_journal_print (LOG_INFO, "Starting freeze/thaw cycle for boot");
-  start_msec = g_get_monotonic_time () / 1000;
+  guint64 start_msec = g_get_monotonic_time () / 1000;
   if (!fsfreeze_thaw_cycle (self, self->boot_fd, cancellable, error))
     return FALSE;
-  end_msec = g_get_monotonic_time () / 1000;
+  guint64 end_msec = g_get_monotonic_time () / 1000;
   ot_journal_print (LOG_INFO, "Completed freeze/thaw cycle for boot in %" G_GUINT64_FORMAT " ms",
                     end_msec - start_msec);
-  out_stats->boot_syncfs_msec = (end_msec - start_msec);
 
   return TRUE;
 }
@@ -2410,8 +2397,7 @@ ostree_sysroot_write_deployments (OstreeSysroot *self, GPtrArray *new_deployment
 static gboolean
 write_deployments_bootswap (OstreeSysroot *self, GPtrArray *new_deployments,
                             OstreeSysrootWriteDeploymentsOpts *opts, OstreeBootloader *bootloader,
-                            SyncStats *out_syncstats, char **out_subbootdir,
-                            GCancellable *cancellable, GError **error)
+                            char **out_subbootdir, GCancellable *cancellable, GError **error)
 {
   const int new_bootversion = self->bootversion ? 0 : 1;
 
@@ -2464,7 +2450,7 @@ write_deployments_bootswap (OstreeSysroot *self, GPtrArray *new_deployments,
   if (!prepare_new_bootloader_link (self, self->bootversion, new_bootversion, cancellable, error))
     return FALSE;
 
-  if (!full_system_sync (self, out_syncstats, cancellable, error))
+  if (!full_system_sync (self, cancellable, error))
     return FALSE;
 
   if (!swap_bootloader (self, bootloader, self->bootversion, new_bootversion, cancellable, error))
@@ -2994,9 +2980,6 @@ ostree_sysroot_write_deployments_with_options (OstreeSysroot *self, GPtrArray *n
     return glnx_throw (error, "Attempting to remove booted deployment");
 
   gboolean bootloader_is_atomic = FALSE;
-  SyncStats syncstats = {
-    0,
-  };
   g_autoptr (OstreeBootloader) bootloader = NULL;
   g_autofree char *new_subbootdir = NULL;
   if (!requires_new_bootversion)
@@ -3004,7 +2987,7 @@ ostree_sysroot_write_deployments_with_options (OstreeSysroot *self, GPtrArray *n
       if (!create_new_bootlinks (self, self->bootversion, new_deployments, cancellable, error))
         return FALSE;
 
-      if (!full_system_sync (self, &syncstats, cancellable, error))
+      if (!full_system_sync (self, cancellable, error))
         return FALSE;
 
       if (!swap_bootlinks (self, self->bootversion, new_deployments, &new_subbootdir, cancellable,
@@ -3020,8 +3003,8 @@ ostree_sysroot_write_deployments_with_options (OstreeSysroot *self, GPtrArray *n
 
       bootloader_is_atomic = bootloader != NULL && _ostree_bootloader_is_atomic (bootloader);
 
-      if (!write_deployments_bootswap (self, new_deployments, opts, bootloader, &syncstats,
-                                       &new_subbootdir, cancellable, error))
+      if (!write_deployments_bootswap (self, new_deployments, opts, bootloader, &new_subbootdir,
+                                       cancellable, error))
         return FALSE;
     }
 
@@ -3032,15 +3015,14 @@ ostree_sysroot_write_deployments_with_options (OstreeSysroot *self, GPtrArray *n
                            requires_new_bootversion ? "yes" : "no", new_subbootdir,
                            new_deployments->len - self->deployments->len);
     const gchar *bootloader_config = ostree_repo_get_bootloader (ostree_sysroot_repo (self));
-    ot_journal_send (
-        "MESSAGE_ID=" SD_ID128_FORMAT_STR, SD_ID128_FORMAT_VAL (OSTREE_DEPLOYMENT_COMPLETE_ID),
-        "MESSAGE=%s", msg, "OSTREE_BOOTLOADER=%s",
-        bootloader ? _ostree_bootloader_get_name (bootloader) : "none",
-        "OSTREE_BOOTLOADER_CONFIG=%s", bootloader_config, "OSTREE_BOOTLOADER_ATOMIC=%s",
-        bootloader_is_atomic ? "yes" : "no", "OSTREE_DID_BOOTSWAP=%s",
-        requires_new_bootversion ? "yes" : "no", "OSTREE_N_DEPLOYMENTS=%u", new_deployments->len,
-        "OSTREE_SYNCFS_ROOT_MSEC=%" G_GUINT64_FORMAT, syncstats.root_syncfs_msec,
-        "OSTREE_SYNCFS_BOOT_MSEC=%" G_GUINT64_FORMAT, syncstats.boot_syncfs_msec, NULL);
+    ot_journal_send ("MESSAGE_ID=" SD_ID128_FORMAT_STR,
+                     SD_ID128_FORMAT_VAL (OSTREE_DEPLOYMENT_COMPLETE_ID), "MESSAGE=%s", msg,
+                     "OSTREE_BOOTLOADER=%s",
+                     bootloader ? _ostree_bootloader_get_name (bootloader) : "none",
+                     "OSTREE_BOOTLOADER_CONFIG=%s", bootloader_config,
+                     "OSTREE_BOOTLOADER_ATOMIC=%s", bootloader_is_atomic ? "yes" : "no",
+                     "OSTREE_DID_BOOTSWAP=%s", requires_new_bootversion ? "yes" : "no",
+                     "OSTREE_N_DEPLOYMENTS=%u", new_deployments->len, NULL);
     _ostree_sysroot_emit_journal_msg (self, msg);
   }
 

diff --git a/src/libostree/ostree-sysroot.c b/src/libostree/ostree-sysroot.c
index 40498411fa467f746f989869134bca572ede9469..282df6d100c709c121d46b180d112cbd23e1c42f 100644 (file)
--- a/src/libostree/ostree-sysroot.c
+++ b/src/libostree/ostree-sysroot.c
@@ -1076,10 +1076,30 @@ ensure_repo (OstreeSysroot *self, GError **error)
     return TRUE;
   if (!ensure_sysroot_fd (self, error))
     return FALSE;
+  // TODO: Consider if we can use openat2 with RESOLVE_NO_XDEV
   self->repo = ostree_repo_open_at (self->sysroot_fd, "ostree/repo", NULL, error);
   if (!self->repo)
     return FALSE;
 
+  // Because the ostree model requires hardlinks, ensure up front
+  // here that /ostree is on the same filesystem as /ostree/repo.
+  // See `full_system_sync` which also requires this.
+  {
+    struct stat repo_stat, ostree_stat;
+    glnx_autofd int ostree_fd = -1;
+    if (!glnx_opendirat (self->sysroot_fd, "ostree", TRUE, &ostree_fd, error))
+      return FALSE;
+    if (!glnx_fstat (ostree_fd, &ostree_stat, error))
+      return FALSE;
+    if (!glnx_fstat (ostree_repo_get_dfd (self->repo), &repo_stat, error))
+      return FALSE;
+    if (ostree_stat.st_dev != repo_stat.st_dev)
+      return glnx_throw (error,
+                         "Unexpected state: ostree/ on device %" G_GUINT64_FORMAT
+                         " but ostree/repo on device %" G_GUINT64_FORMAT,
+                         (guint64)ostree_stat.st_dev, (guint64)repo_stat.st_dev);
+  }
+
   /* Flag it as having been created via ostree_sysroot_get_repo(), and hold a
    * weak ref for the remote-add handling.
    */

diff --git a/tests/kolainst/destructive/staged-deploy.sh b/tests/kolainst/destructive/staged-deploy.sh
index e2fe1b54f87484562374e39fab9810fa586c51e2..184da62cd111216039d3df0c5f16d5c8aed1ec8a 100755 (executable)
--- a/tests/kolainst/destructive/staged-deploy.sh
+++ b/tests/kolainst/destructive/staged-deploy.sh
@@ -54,6 +54,9 @@ EOF
     orig_mtime=$(stat -c '%.Y' /sysroot/ostree/deploy)
     systemctl show -p ActiveState ostree-finalize-staged.service | grep -q inactive
 
+    syncfs=$(journalctl --grep='Completed syncfs.*for system repo' | wc -l)
+    assert_streq "$syncfs" 2
+
     # Do the staged deployment
     ostree admin deploy --stage staged-deploy
 
@@ -91,6 +94,10 @@ EOF
     # And there should not be a staged deployment
     test '!' -f /run/ostree/staged-deployment
 
+    # Also verify we did a syncfs run during finalization
+    syncfs=$(journalctl -b -1 -u ostree-finalize-staged --grep='Completed syncfs.*for system repo' | wc -l)
+    assert_streq "$syncfs" 2
+
     test '!' -f /run/ostree/staged-deployment
     ostree admin status > status.txt
     assert_not_file_has_content status.txt 'finalization locked'